http2: add failing writes with destroy test #19852

mafintosh · 2018-04-06T15:54:02Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Simplified the failing http2 test from #19828 into this test case. Basically when doing lots of writes and then destroying the request, the process will hang instead of exiting normally.

Fixes #20630

mafintosh · 2018-04-06T15:54:27Z

/cc @jasnell @addaleax

addaleax

This would need to be rebased due to #19854, sorry :/

addaleax · 2018-04-06T16:58:10Z

test/parallel/test-http2-many-writes-and-destroy.js

+
+  setTimeout(() => {
+    assert(false, 'should exit before 10s');
+  }, 10000).unref();


I think we've been removing these kinds of checks to avoid timeout-related flakiness in tests -- the test runner enforces its own timeout anyway. (/cc @Trott)

If you want to have actual timing guarantees, you can try to remove the dependency on networking and use something like test/parallel/test-http2-generic-streams.js does.

Other things to be aware of, just sorta FYI, you don't need to do anything with these at this time unless you think you should:

Tests with timers like this can become unreliable if the test is competing with other tests for resources. This unreliability is introduced surprisingly often on a few platforms in CI (particularly FreeBSD, I think because test.py decides it has 48 processors or something but they're not "real" processors--never really dug into it TBH, but you get the idea). To avoid the problem, the test can be put in sequential rather than parallel.

If you think 10 seconds is plenty for most things, but that maybe a Raspberry Pi could use a little more time than that, use common.platformTimeout().

addaleax · 2018-04-06T16:58:39Z

test/parallel/test-http2-many-writes-and-destroy.js

+      client.close();
+    }));
+
+    req.once('data', () => req.destroy());


I think this could be common.mustCallAtLeast()'ed, to make sure it's run?

Since it's a once handler, it could be common.mustCall().

If the test will fail without the handler running, then I'm OK omitting common.mustCall(). And I'm OK including it. Either way...

fixed. added a mustCall, since it's a once.

Trott · 2018-04-06T22:05:31Z

This would need to be rebased due to #19854, sorry :/

I did the rebase, so no need to worry about that anymore.

Trott · 2018-04-06T22:06:01Z

@nodejs/http2 @nodejs/testing

BridgeAR · 2018-04-08T17:16:36Z

CI https://ci.nodejs.org/job/node-test-pull-request/14130/

mafintosh · 2018-04-09T10:00:29Z

Fixed the comments. PTAL @addaleax @Trott @jasnell

mcollina · 2018-04-11T14:55:44Z

This test is failing and it is showing a bug :/.

I've run why-is-node-running with it, and we just have:

There are 7 handle(s) keeping the process running

# Timeout
/Users/matteo/Repositories/node/test/parallel/test-http2-many-writes-and-destroy.js:35 - setTimeout(function () {

# TCPWRAP
/Users/matteo/Repositories/node/test/parallel/test-http2-many-writes-and-destroy.js:17 - const client = http2.connect(url);

# HTTP2SESSION
(unknown stack trace)

# HTTP2SETTINGS
(unknown stack trace)

# Timeout
(unknown stack trace)

# HTTP2SESSION
(unknown stack trace)

# HTTP2STREAM
(unknown stack trace)

I have a possible fix coming, pushing directly to this PR.

mafintosh · 2018-04-11T15:43:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14205/

mafintosh · 2018-04-11T15:43:38Z

🎉 @mcollina added a fix for my failing test case \o/

jasnell · 2018-04-11T15:53:36Z

lib/internal/http2/core.js

@@ -1190,7 +1190,7 @@ class Http2Session extends EventEmitter {
    // Otherwise, destroy immediately.
    if (!socket.destroyed) {
      if (!error) {
-        setImmediate(socket.end.bind(socket));
+        setImmediate(socket.destroy.bind(socket));


oy! obvious and simple in hindsight.

a classic @mcollina fix :)

jasnell · 2018-04-11T15:54:06Z

LGTM, especially with the fix :-)

BridgeAR · 2018-04-13T18:23:57Z

Rerunning the CI because there were a couple of failures https://ci.nodejs.org/job/node-test-pull-request/14265/

addaleax · 2018-04-14T16:07:58Z

@mafintosh Can you make sure that the CI failures are unrelated? In particular, at a quick glance https://ci.nodejs.org/job/node-test-commit-osx/17798/nodes=osx1010/console might not be…

apapirovski · 2018-04-24T07:04:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14651/

mcollina · 2018-05-10T12:42:35Z

@apapirovski I've added your commit on top.

mcollina · 2018-05-10T12:43:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14786/

apapirovski · 2018-05-10T14:20:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14792/

I added some other fixes for the new changes. I also removed the changes to the http2 pipe test because if that one's getting an ECONNRESET then we shouldn't swallow it. That would indicate the fix here isn't fully correct (e.g. it's possible for us to be still writing meaningful data when the socket is destroyed).

apapirovski · 2018-05-11T04:46:35Z

Ok, the CI is green FWIW. If someone wants to give this a few more looks then we could go ahead and finally land this.

(Another CI just to be safe: https://ci.nodejs.org/job/node-test-pull-request/14805/)

mcollina · 2018-05-11T22:01:33Z

@jasnell @addaleax @trivikr PTAL

jasnell · 2018-05-12T23:05:44Z

Still LGTM

Fix a bug where the socket wasn't being correctly destroyed and adjust existing tests, as well as add additional tests. PR-URL: #19852 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

apapirovski · 2018-05-13T17:50:57Z

Landed in b60b183

Fix a bug where the socket wasn't being correctly destroyed and adjust existing tests, as well as add additional tests. PR-URL: #19852 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 6, 2018

mafintosh added the http2 Issues or PRs related to the http2 subsystem. label Apr 6, 2018

addaleax force-pushed the master branch from 4ae53af to 3dc5404 Compare April 6, 2018 16:54

addaleax approved these changes Apr 6, 2018

View reviewed changes

Trott force-pushed the add-failing-http2-test-with-writes branch from be13bfe to 5b55170 Compare April 6, 2018 22:05

mafintosh force-pushed the add-failing-http2-test-with-writes branch from 5b55170 to 17630b1 Compare April 9, 2018 09:58

mafintosh force-pushed the add-failing-http2-test-with-writes branch from 17630b1 to cf5a405 Compare April 9, 2018 11:26

jasnell approved these changes Apr 9, 2018

View reviewed changes

mcollina force-pushed the add-failing-http2-test-with-writes branch from cf5a405 to 9cc9a9d Compare April 11, 2018 15:39

jasnell reviewed Apr 11, 2018

View reviewed changes

trivikr approved these changes Apr 11, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2018

mcollina mentioned this pull request Apr 16, 2018

difference between http2 Compatibility API and http #20060

Closed

mcollina mentioned this pull request Apr 24, 2018

streamError never emitted #20211

Closed

apapirovski mentioned this pull request May 10, 2018

http2: server.close not behaving like http1 or net #20630

Closed

mafintosh and others added 2 commits May 10, 2018 08:54

http2: add failing writes with destroy test

2df7396

http2: destroy the underlining socket on destroy

e7b7035

apapirovski force-pushed the add-failing-http2-test-with-writes branch from e3f9922 to c3c1f3d Compare May 10, 2018 14:18

apapirovski force-pushed the add-failing-http2-test-with-writes branch 2 times, most recently from 56c994c to a402353 Compare May 10, 2018 14:44

test: fix flaky http2 tests

53eb584

apapirovski force-pushed the add-failing-http2-test-with-writes branch from a402353 to 53eb584 Compare May 10, 2018 14:55

trivikr approved these changes May 12, 2018

View reviewed changes

apapirovski closed this May 13, 2018

addaleax mentioned this pull request May 14, 2018

v10.2.0 proposal #20724

Merged

addaleax added the land-on-v8.x label Jun 29, 2018

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Trott mentioned this pull request Nov 25, 2018

meta: patch branches #24467

Closed

lundibundi mentioned this pull request Dec 8, 2019

http2: fix graceful session close #30854

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: add failing writes with destroy test #19852

http2: add failing writes with destroy test #19852

mafintosh commented Apr 6, 2018 •

edited by mcollina

Loading

mafintosh commented Apr 6, 2018

addaleax left a comment

addaleax Apr 6, 2018

Trott Apr 6, 2018

mafintosh Apr 9, 2018

addaleax Apr 6, 2018

Trott Apr 6, 2018

mafintosh Apr 9, 2018

Trott commented Apr 6, 2018

Trott commented Apr 6, 2018

BridgeAR commented Apr 8, 2018

mafintosh commented Apr 9, 2018 •

edited

Loading

mcollina commented Apr 11, 2018

mafintosh commented Apr 11, 2018

mafintosh commented Apr 11, 2018

jasnell Apr 11, 2018

mafintosh Apr 11, 2018

jasnell commented Apr 11, 2018

BridgeAR commented Apr 13, 2018

addaleax commented Apr 14, 2018

apapirovski commented Apr 24, 2018 •

edited

Loading

mcollina commented May 10, 2018

mcollina commented May 10, 2018

apapirovski commented May 10, 2018 •

edited

Loading

apapirovski commented May 11, 2018

mcollina commented May 11, 2018

jasnell commented May 12, 2018

apapirovski commented May 13, 2018

http2: add failing writes with destroy test #19852

http2: add failing writes with destroy test #19852

Conversation

mafintosh commented Apr 6, 2018 • edited by mcollina Loading

Checklist

mafintosh commented Apr 6, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Apr 6, 2018

Trott commented Apr 6, 2018

BridgeAR commented Apr 8, 2018

mafintosh commented Apr 9, 2018 • edited Loading

mcollina commented Apr 11, 2018

mafintosh commented Apr 11, 2018

mafintosh commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 11, 2018

BridgeAR commented Apr 13, 2018

addaleax commented Apr 14, 2018

apapirovski commented Apr 24, 2018 • edited Loading

mcollina commented May 10, 2018

mcollina commented May 10, 2018

apapirovski commented May 10, 2018 • edited Loading

apapirovski commented May 11, 2018

mcollina commented May 11, 2018

jasnell commented May 12, 2018

apapirovski commented May 13, 2018

mafintosh commented Apr 6, 2018 •

edited by mcollina

Loading

mafintosh commented Apr 9, 2018 •

edited

Loading

apapirovski commented Apr 24, 2018 •

edited

Loading

apapirovski commented May 10, 2018 •

edited

Loading